fix(isHashMatch): check that hash starts with $ #3698
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3698 +/- ##
==========================================
+ Coverage 60.13% 60.22% +0.08%
==========================================
Files 35 35
Lines 1811 1810 -1
Branches 365 365
==========================================
+ Hits 1089 1090 +1
+ Misses 606 604 -2
Partials 116 116
Continue to review full report at Codecov.
|
Previously, we used argon2 to verify the hash with the password. If the hash didn't start with a $, then it would enter the catch block. Now we check the hash before trying to verify it and we also throw an Error if the verify fails. This makes the isHashMatch function more robust.
jsjoeio
force-pushed
the
jsjoeio-fix-argon-issue
branch
from
0e6096e to
7f12fab
Compare
jsjoeio
added
chore
| expect(await util.isHashMatch(password, _hash)).toBe(false) | ||
| }) | ||
| it("should reject the promise and throw if error", async () => { | ||
| const password = "hellowpasssword" |
There was a problem hiding this comment.
Just curious - what does it mean when the hash begins with a $? Is it used as a signifier to designate that the hash is Argon2? I had thought that the other test case was an Argon2 hash, too, so this is a little confusing for me
There was a problem hiding this comment.
Just curious - what does it mean when the hash begins with a $? Is it used as a signifier to designate that the hash is Argon2?
AFAIK, I believe that's exactly why.
had thought that the other test case was an Argon2 hash, too, so this is a little confusing for me
If you point out which test case you're referring to, happening to clear up any confusion!
There was a problem hiding this comment.
Sorry I meant that this:
const _hash = "n2i$v=19$m=4096,t=3,p=1$EAoczTxVki21JDfIZpTUxg$rkXgyrW4RDGoDYrxBFD4H2DlSMEhP4h+Api1hXnGnFY"
Looks like a valid hash to me, but I guess it's not? What is this hash?
I guess it makes sense that it has to start with $ since a valid hash looks like const _hash = "$argon2i$v=19$m=4096,t=3,p=1$EAoczTxVki21JDfIZpTUxg$rkXgyrW4RDGoDYrxBFD4H2DlSMEhP4h+Api1hXnGnFY"?
Should we look for "$argon2i$" instead of just the $?
My fault. I updated the PR description with the why behind this. |
|
Side note: @code-asher brought up a good point about UX here so I opened another issue: #3704 |
This PR adds an additional check to
isHashMatchto check that the hash starts with a "$" to prevent us from callingargon2.verifyif it doesn't.It also throws an error if something goes wrong with the
.verify()function.Why do we need to add this?
The other day I noticed this error:
'TypeError: pchstr must contain a $ as first char\n'I believe it happens because we're still in between a migration from the old hashing function
sha256to the new oneargon2. And locally, I switch between the latest code-server and the development one. This means it may be calling the function to verify the hash expecting it to be a hash made withargon2and instead it's made withsha256.Once we release the newest version of code-server with the
argon2algorithm, we don't want it to throw unexpected errors, like this one. We may need to revisit some of the logic for verifying password hashes in case there are issues.Either way, this adds an extra check to prevent this error from happening.
Fixes N/A